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 14 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
97 changes: 24 additions & 73 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;

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

View workflow job for this annotation

GitHub Actions / analyze / analyze

'beforeScreenshot' is deprecated and shouldn't be used. Use `screenshot.beforeScreenshot` instead.

Try replacing the use of the deprecated member with the replacement. See https://dart.dev/diagnostics/deprecated_member_use_from_same_package to learn more about this problem.
if (beforeScreenshot != null) {
try {
final result = beforeScreenshot(event, hint: hint);
Expand Down Expand Up @@ -67,7 +67,7 @@
return event;
}

if (_options.attachScreenshotOnlyWhenResumed &&

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

View workflow job for this annotation

GitHub Actions / analyze / analyze

'attachScreenshotOnlyWhenResumed' is deprecated and shouldn't be used. Use `screenshot.attachScreenshotOnlyWhenResumed` instead.

Try replacing the use of the deprecated member with the replacement. See https://dart.dev/diagnostics/deprecated_member_use_from_same_package to learn more about this problem.
widget.WidgetsBinding.instance.lifecycleState !=
AppLifecycleState.resumed) {
_options.logger(SentryLevel.debug,
Expand All @@ -75,83 +75,34 @@
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.screenshotQuality),

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

View workflow job for this annotation

GitHub Actions / analyze / analyze

'screenshotQuality' is deprecated and shouldn't be used. Use `screenshot.screenshotQuality` instead.

Try replacing the use of the deprecated member with the replacement. See https://dart.dev/diagnostics/deprecated_member_use_from_same_package to learn more about this problem.
_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 103 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#L103

Added line #L103 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/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();
}
}
}
}
15 changes: 15 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,19 @@
options.sdk = sdk;
}

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 255 in flutter/lib/src/sentry_flutter.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/sentry_flutter.dart#L252-L255

Added lines #L252 - L255 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