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

redact screenshots via view hierarchy #2361

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Enhancements

- Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365))
- Switching from traditional screenshot to view hierarchy for screenshots which allows redacting ([#2361](https://github.com/getsentry/sentry-dart/pull/2361))
Copy link
Contributor

Choose a reason for hiding this comment

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

the wording is a bit confusing from a user perspective we can improve it by sayingAllow Screenshots to be masked for privacy reasons or something like that

Also show an exampl how a user can enable it and please add the screenshot & replay masking behaviour (the one that I commented) as sub bullet points for more context so it's clear how it behaves

Comment on lines 5 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry should be under ## Features, if there is none please create it


## 8.10.0-beta.2

Expand Down
3 changes: 2 additions & 1 deletion flutter/lib/sentry_flutter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ export 'src/navigation/sentry_navigator_observer.dart';
export 'src/sentry_flutter.dart';
export 'src/sentry_flutter_options.dart';
export 'src/sentry_replay_options.dart';
export 'src/sentry_privacy_options.dart';
export 'src/flutter_sentry_attachment.dart';
export 'src/sentry_asset_bundle.dart' show SentryAssetBundle;
export 'src/integrations/on_error_integration.dart';
export 'src/replay/masking_config.dart' show SentryMaskingDecision;
export 'src/screenshot/masking_config.dart' show SentryMaskingDecision;
export 'src/screenshot/sentry_mask_widget.dart';
export 'src/screenshot/sentry_unmask_widget.dart';
export 'src/screenshot/sentry_screenshot_widget.dart';
Expand Down
102 changes: 27 additions & 75 deletions flutter/lib/src/event_processor/screenshot_event_processor.dart
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import 'dart:async';
import 'dart:math';
import 'dart:typed_data';
import 'dart:ui';

import 'package:sentry/sentry.dart';
import '../screenshot/recorder.dart';
import '../screenshot/recorder_config.dart';
import '../screenshot/sentry_screenshot_widget.dart';
import '../sentry_flutter_options.dart';
import 'package:flutter/rendering.dart';
import '../renderer/renderer.dart';
import 'package:flutter/widgets.dart' as widget;

Expand All @@ -30,7 +30,7 @@
_hasSentryScreenshotWidget) {
return event;
}
final beforeScreenshot = _options.beforeScreenshot;
final beforeScreenshot = _options.screenshot.beforeScreenshot;
if (beforeScreenshot != null) {
try {
final result = beforeScreenshot(event, hint: hint);
Expand Down Expand Up @@ -67,91 +67,43 @@
return event;
}

if (_options.attachScreenshotOnlyWhenResumed &&
if (_options.screenshot.attachScreenshotOnlyWhenResumed &&
widget.WidgetsBinding.instance.lifecycleState !=
AppLifecycleState.resumed) {
_options.logger(SentryLevel.debug,
'Only attaching screenshots when application state is resumed.');
return event;
}

final bytes = await _createScreenshot();
if (bytes != null) {
hint.screenshot = SentryAttachment.fromScreenshotData(bytes);
}
return event;
}
// ignore: deprecated_member_use
var recorder = ScreenshotRecorder(
ScreenshotRecorderConfig(
quality: _options.screenshot.screenshotQuality),
_options);

Future<Uint8List?> _createScreenshot() async {
try {
final renderObject =
sentryScreenshotWidgetGlobalKey.currentContext?.findRenderObject();
if (renderObject is RenderRepaintBoundary) {
// ignore: deprecated_member_use
final pixelRatio = window.devicePixelRatio;
var imageResult = _getImage(renderObject, pixelRatio);
Image image;
if (imageResult is Future<Image>) {
image = await imageResult;
} else {
image = imageResult;
}
// At the time of writing there's no other image format available which
// Sentry understands.
Uint8List? _screenshotData;

if (image.width == 0 || image.height == 0) {
_options.logger(SentryLevel.debug,
'View\'s width and height is zeroed, not taking screenshot.');
return null;
}

final targetResolution = _options.screenshotQuality.targetResolution();
if (targetResolution != null) {
var ratioWidth = targetResolution / image.width;
var ratioHeight = targetResolution / image.height;
var ratio = min(ratioWidth, ratioHeight);
if (ratio > 0.0 && ratio < 1.0) {
imageResult = _getImage(renderObject, ratio * pixelRatio);
if (imageResult is Future<Image>) {
image = await imageResult;
} else {
image = imageResult;
}
}
}
final byteData = await image.toByteData(format: ImageByteFormat.png);
await recorder.capture((Image image) async {
_screenshotData = await _convertImageToUint8List(image);
});

final bytes = byteData?.buffer.asUint8List();
if (bytes?.isNotEmpty == true) {
return bytes;
} else {
_options.logger(SentryLevel.debug,
'Screenshot is 0 bytes, not attaching the image.');
return null;
}
}
} catch (exception, stackTrace) {
_options.logger(
SentryLevel.error,
'Taking screenshot failed.',
exception: exception,
stackTrace: stackTrace,
);
if (_options.automatedTestMode) {
rethrow;
}
if (_screenshotData != null) {
hint.screenshot = SentryAttachment.fromScreenshotData(_screenshotData!);
}
return null;

return event;
}

FutureOr<Image> _getImage(
RenderRepaintBoundary repaintBoundary, double pixelRatio) {
// This one is a hack to use https://api.flutter.dev/flutter/rendering/RenderRepaintBoundary/toImage.html on versions older than 3.7 and https://api.flutter.dev/flutter/rendering/RenderRepaintBoundary/toImageSync.html on versions equal or newer than 3.7
try {
return (repaintBoundary as dynamic).toImageSync(pixelRatio: pixelRatio)
as Image;
} on NoSuchMethodError catch (_) {
return repaintBoundary.toImage(pixelRatio: pixelRatio);
Future<Uint8List?> _convertImageToUint8List(Image image) async {
final byteData = await image.toByteData(format: ImageByteFormat.png);

final bytes = byteData?.buffer.asUint8List();
if (bytes?.isNotEmpty == true) {
return bytes;
} else {
_options.logger(

Check warning on line 104 in flutter/lib/src/event_processor/screenshot_event_processor.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/event_processor/screenshot_event_processor.dart#L104

Added line #L104 was not covered by tests
SentryLevel.debug, 'Screenshot is 0 bytes, not attaching the image.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ever happened to you while testing? Looks like a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, not that I am aware of. This code was already here before.
Maybe @denrase knows more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vaind I looked up the toByteData method, and it will return null if an error occurs and also throws an exception. I would rewrite this, with a try/catch block, but we can also remove this code, if you think, that we don`t need it. WDYT?

return null;
}
}
}
4 changes: 2 additions & 2 deletions flutter/lib/src/integrations/screenshot_integration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import '../event_processor/screenshot_event_processor.dart';
import '../sentry_flutter_options.dart';

/// Adds [ScreenshotEventProcessor] to options event processors if
/// [SentryFlutterOptions.attachScreenshot] is true
/// [SentryFlutterOptions.screenshot.attachScreenshot] is true
class ScreenshotIntegration implements Integration<SentryFlutterOptions> {
SentryFlutterOptions? _options;
ScreenshotEventProcessor? _screenshotEventProcessor;

@override
void call(Hub hub, SentryFlutterOptions options) {
if (options.attachScreenshot) {
if (options.screenshot.attachScreenshot) {
_options = options;
final screenshotEventProcessor = ScreenshotEventProcessor(options);
options.addEventProcessor(screenshotEventProcessor);
Expand Down
4 changes: 2 additions & 2 deletions flutter/lib/src/native/cocoa/sentry_native_cocoa.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import 'package:meta/meta.dart';

import '../../../sentry_flutter.dart';
import '../../event_processor/replay_event_processor.dart';
import '../../screenshot/recorder.dart';
import '../../screenshot/recorder_config.dart';
import '../../replay/integration.dart';
import '../../replay/recorder.dart';
import '../../replay/recorder_config.dart';
import '../sentry_native_channel.dart';
import 'binding.dart' as cocoa;

Expand Down
2 changes: 1 addition & 1 deletion flutter/lib/src/native/java/sentry_native_java.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import '../../../sentry_flutter.dart';
import '../../event_processor/replay_event_processor.dart';
import '../../replay/integration.dart';
import '../../replay/scheduled_recorder.dart';
import '../../replay/recorder_config.dart';
import '../../replay/scheduled_recorder_config.dart';
import '../sentry_native_channel.dart';

// Note: currently this doesn't do anything. Later, it shall be used with
Expand Down
4 changes: 2 additions & 2 deletions flutter/lib/src/replay/scheduled_recorder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ import 'dart:async';
import 'dart:ui';

import 'package:meta/meta.dart';
import 'scheduled_recorder_config.dart';

import '../../sentry_flutter.dart';
import 'recorder.dart';
import 'recorder_config.dart';
import '../screenshot/recorder.dart';
import 'scheduler.dart';

@internal
Expand Down
11 changes: 11 additions & 0 deletions flutter/lib/src/replay/scheduled_recorder_config.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import '../screenshot/recorder_config.dart';

class ScheduledScreenshotRecorderConfig extends ScreenshotRecorderConfig {
final int frameRate;

const ScheduledScreenshotRecorderConfig({
super.width,
super.height,
required this.frameRate,
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:flutter/rendering.dart';
import 'package:meta/meta.dart';

import '../../sentry_flutter.dart';
import 'masking_config.dart';
import 'recorder_config.dart';
import 'widget_filter.dart';

Expand All @@ -21,7 +22,11 @@ class ScreenshotRecorder {
bool warningLogged = false;

ScreenshotRecorder(this.config, this.options) {
final maskingConfig = options.experimental.replay.buildMaskingConfig();
/// TODO: Rewrite when default redaction value are synced with SS & SR
final SentryMaskingConfig maskingConfig =
(options.experimental.privacy ?? SentryPrivacyOptions())
.buildMaskingConfig();

if (maskingConfig.length > 0) {
_widgetFilter = WidgetFilter(maskingConfig, options.logger);
}
Expand Down Expand Up @@ -82,12 +87,16 @@ class ScreenshotRecorder {
final picture = recorder.endRecording();

try {
final finalImage = await picture.toImage(
(srcWidth * pixelRatio).round(), (srcHeight * pixelRatio).round());
Image finalImage;
final targetHeight =
config.quality.calculateHeight(srcWidth.toInt(), srcHeight.toInt());
final targetWidth =
config.quality.calculateWidth(srcWidth.toInt(), srcHeight.toInt());
finalImage = await picture.toImage(targetWidth, targetHeight);
try {
await callback(finalImage);
} finally {
finalImage.dispose();
finalImage.dispose(); // image needs to be disposed manually
}
} finally {
picture.dispose();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@ import 'dart:math';

import 'package:meta/meta.dart';

import '../../sentry_flutter.dart';

@internal
class ScreenshotRecorderConfig {
final int? width;
final int? height;
final SentryScreenshotQuality quality;

const ScreenshotRecorderConfig({this.width, this.height});
const ScreenshotRecorderConfig({
this.width,
this.height,
this.quality = SentryScreenshotQuality.full,
});

double getPixelRatio(double srcWidth, double srcHeight) {
assert((width == null) == (height == null));
Expand All @@ -17,13 +24,3 @@ class ScreenshotRecorderConfig {
return min(width! / srcWidth, height! / srcHeight);
}
}

class ScheduledScreenshotRecorderConfig extends ScreenshotRecorderConfig {
final int frameRate;

const ScheduledScreenshotRecorderConfig({
super.width,
super.height,
required this.frameRate,
});
}
30 changes: 30 additions & 0 deletions flutter/lib/src/screenshot/sentry_screenshot_quality.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'package:meta/meta.dart';

/// The quality of the attached screenshot
enum SentryScreenshotQuality {
full,
Expand All @@ -17,4 +19,32 @@ enum SentryScreenshotQuality {
return 854;
}
}

@internal
int calculateHeight(int width, int height) {
martinhaintz marked this conversation as resolved.
Show resolved Hide resolved
if (this == SentryScreenshotQuality.full) {
return height;
} else {
if (height > width) {
return targetResolution()!;
} else {
var ratio = targetResolution()! / width;
return (height * ratio).round();
}
}
}

@internal
int calculateWidth(int width, int height) {
if (this == SentryScreenshotQuality.full) {
return width;
} else {
if (width > height) {
return targetResolution()!;
} else {
var ratio = targetResolution()! / height;
return (width * ratio).round();
}
}
}
}
23 changes: 23 additions & 0 deletions flutter/lib/src/sentry_flutter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@
// ignore: invalid_use_of_internal_member
runZonedGuardedOnError: runZonedGuardedOnError,
);
// TODO: Remove when we synced SS and SR configurations and have a single default configuration
_setRedactionOptions(options);

if (_native != null) {
// ignore: invalid_use_of_internal_member
Expand Down Expand Up @@ -243,6 +245,27 @@
options.sdk = sdk;
}

/// Screen redaction was previously introduced with the SessionReplay feature.
/// Screen redaction is enabled by default for SessionReplay.
/// As we also to use this feature for Screenshot, which previously was not
/// capable of redacting the screenshot, we need to disable redaction for Screenshot by default
/// so we don`t break the existing behavior.
/// As we have only one central place to configure the redaction,
/// we need to set the redaction options to full fill the above default settings.
/// The plan is to unify this behaviour with the next major release.
Comment on lines +248 to +255
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Screen redaction was previously introduced with the SessionReplay feature.
/// Screen redaction is enabled by default for SessionReplay.
/// As we also to use this feature for Screenshot, which previously was not
/// capable of redacting the screenshot, we need to disable redaction for Screenshot by default
/// so we don`t break the existing behavior.
/// As we have only one central place to configure the redaction,
/// we need to set the redaction options to full fill the above default settings.
/// The plan is to unify this behaviour with the next major release.
/// Masking behaviour
/// - If only Screenshot is enabled: masking is disabled by default.
/// - If both Screenshot and Session Replay are enabled: masking is enabled for both by default.
/// - If the user explicitly sets masking to false: masking is disabled for both features.
/// We don't want to break the existing screenshot integration which is not masked by default.
/// The plan is to unify screenshot and replay masking with the next major release.

suggestion to keep it a bit shorter

static void _setRedactionOptions(SentryFlutterOptions options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static void _setRedactionOptions(SentryFlutterOptions options) {
static void _setMaskingOptions(SentryFlutterOptions options) {

minor: since we changed the wording from redact -> mask, it might make sense to change it

if (options.experimental.privacy != null) {
return;
} else if (options.screenshot.attachScreenshot == true &&
!options.experimental.replay.isEnabled) {
options.experimental.privacy = SentryPrivacyOptions()
..maskAllText = false
..maskAllImages = false;

Check warning on line 263 in flutter/lib/src/sentry_flutter.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/sentry_flutter.dart#L260-L263

Added lines #L260 - L263 were not covered by tests
} else {
options.experimental.privacy = SentryPrivacyOptions();
}
}
martinhaintz marked this conversation as resolved.
Show resolved Hide resolved

/// Reports the time it took for the screen to be fully displayed.
/// This requires the [SentryFlutterOptions.enableTimeToFullDisplayTracing] option to be set to `true`.
static Future<void> reportFullyDisplayed() async {
Expand Down
Loading
Loading