-
-
Notifications
You must be signed in to change notification settings - Fork 276
replay: add sensitive content widget to default masking #2989
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
base: main
Are you sure you want to change the base?
Changes from all commits
333f93a
a240674
6d1e35e
74eb694
2e01b33
132f050
591b9c3
5ce2839
dfce0e8
efa9362
f76a31d
b928028
f4c8e58
151a70d
c8596cd
dde1132
7c9f696
088cf62
4bbbded
20657ad
32e3fc4
0c59077
686750f
1f8bf63
e38ab52
33b5751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,9 @@ import 'package:meta/meta.dart'; | |
| import '../sentry_flutter.dart'; | ||
| import 'screenshot/masking_config.dart'; | ||
| import 'screenshot/widget_filter.dart'; | ||
| // ignore: implementation_imports | ||
| import 'package:sentry/src/event_processor/enricher/flutter_runtime.dart' | ||
| as flutter_runtime; | ||
|
|
||
| /// Configuration of the experimental privacy feature. | ||
| class SentryPrivacyOptions { | ||
|
|
@@ -75,6 +78,11 @@ class SentryPrivacyOptions { | |
| )); | ||
| } | ||
|
|
||
| const flutterVersion = flutter_runtime.FlutterVersion.version; | ||
| if (flutterVersion != null) { | ||
| _maybeAddSensitiveContentRule(rules, flutterVersion); | ||
| } | ||
|
|
||
| // In Debug mode, check if users explicitly mask (or unmask) widgets that | ||
| // look like they should be masked, e.g. Videos, WebViews, etc. | ||
| if (runtimeChecker.isDebugMode()) { | ||
|
|
@@ -162,6 +170,56 @@ class SentryPrivacyOptions { | |
| } | ||
| } | ||
|
|
||
| /// Returns `true` if a SensitiveContent masking rule _should_ be added for a | ||
| /// given [flutterVersion] string. The SensitiveContent widget was introduced | ||
| /// in Flutter 3.33, therefore we only add the masking rule when the detected | ||
| /// version is >= 3.33. | ||
| bool _shouldAddSensitiveContentRule(String version) { | ||
| final dot = version.indexOf('.'); | ||
| if (dot == -1) return false; | ||
|
|
||
| final major = int.tryParse(version.substring(0, dot)); | ||
| final nextDot = version.indexOf('.', dot + 1); | ||
| final minor = int.tryParse( | ||
| version.substring(dot + 1, nextDot == -1 ? version.length : nextDot)); | ||
|
|
||
| return major != null && | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Could do early returns when either major/minor is null. Also, do we do version check anywhere else? Then we could extract the version reading to a common place and only do the version check here, as we do both now. |
||
| minor != null && | ||
| (major > 3 || (major == 3 && minor >= 33)); | ||
| } | ||
|
|
||
| /// Adds a masking rule for the [SensitiveContent] widget. | ||
| /// | ||
| /// The rule masks any widget that exposes a `sensitivity` property which is an | ||
| /// [Enum]. This is how the [SensitiveContent] widget can be detected | ||
| /// without depending on its type directly (which would fail to compile on | ||
| /// older Flutter versions). | ||
| void _maybeAddSensitiveContentRule( | ||
| List<SentryMaskingRule> rules, String flutterVersion) { | ||
| if (!_shouldAddSensitiveContentRule(flutterVersion)) { | ||
| return; | ||
| } | ||
|
|
||
| SentryMaskingDecision maskSensitiveContent(Element element, Widget widget) { | ||
| try { | ||
| final dynamic dynWidget = widget; | ||
| final sensitivity = dynWidget.sensitivity; | ||
| // If the property exists, we assume this is the SensitiveContent widget. | ||
| assert(sensitivity is Enum); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth to double-check the cursor[bot] comment. |
||
| return SentryMaskingDecision.mask; | ||
| } catch (_) { | ||
| // Property not found – continue processing other rules. | ||
| return SentryMaskingDecision.continueProcessing; | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Masking Behavior Varies Between ModesThe |
||
|
|
||
| rules.add(SentryMaskingCustomRule<Widget>( | ||
| callback: maskSensitiveContent, | ||
| name: 'SensitiveContent', | ||
| description: 'Mask SensitiveContent widget.', | ||
| )); | ||
| } | ||
|
|
||
| SentryMaskingDecision _maskImagesExceptAssets(Element element, Image widget) { | ||
| final image = widget.image; | ||
| if (image is AssetBundleImageProvider) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import 'package:flutter/services.dart'; | ||
| import 'package:flutter/widgets.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:sentry_flutter/sentry_flutter.dart'; | ||
|
|
@@ -125,7 +126,7 @@ void main() async { | |
| SentryMaskingDecision.unmask); | ||
| }); | ||
|
|
||
| testWidgets('retuns false if no rule matches', (tester) async { | ||
| testWidgets('returns false if no rule matches', (tester) async { | ||
| final sut = SentryMaskingConfig([ | ||
| SentryMaskingCustomRule<Image>( | ||
| callback: (e, w) => SentryMaskingDecision.continueProcessing, | ||
|
|
@@ -169,6 +170,7 @@ void main() async { | |
| 'SentryMaskingConstantRule<Text>(mask)', | ||
| 'SentryMaskingConstantRule<EditableText>(mask)', | ||
| 'SentryMaskingConstantRule<RichText>(mask)', | ||
| ..._maybeWithSensitiveContent(), | ||
| 'SentryMaskingCustomRule<Widget>(Debug-mode-only warning for potentially sensitive widgets.)' | ||
| ]); | ||
| }); | ||
|
|
@@ -181,6 +183,7 @@ void main() async { | |
| expect(rulesAsStrings(sut), [ | ||
| ...alwaysEnabledRules, | ||
| 'SentryMaskingConstantRule<Image>(mask)', | ||
| ..._maybeWithSensitiveContent(), | ||
| 'SentryMaskingCustomRule<Widget>(Debug-mode-only warning for potentially sensitive widgets.)' | ||
| ]); | ||
| }); | ||
|
|
@@ -193,6 +196,7 @@ void main() async { | |
| expect(rulesAsStrings(sut), [ | ||
| ...alwaysEnabledRules, | ||
| 'SentryMaskingCustomRule<Image>(Mask all images except asset images.)', | ||
| ..._maybeWithSensitiveContent(), | ||
| 'SentryMaskingCustomRule<Widget>(Debug-mode-only warning for potentially sensitive widgets.)' | ||
| ]); | ||
| }); | ||
|
|
@@ -207,6 +211,7 @@ void main() async { | |
| 'SentryMaskingConstantRule<Text>(mask)', | ||
| 'SentryMaskingConstantRule<EditableText>(mask)', | ||
| 'SentryMaskingConstantRule<RichText>(mask)', | ||
| ..._maybeWithSensitiveContent(), | ||
| 'SentryMaskingCustomRule<Widget>(Debug-mode-only warning for potentially sensitive widgets.)' | ||
| ]); | ||
| }); | ||
|
|
@@ -218,31 +223,66 @@ void main() async { | |
| ..maskAssetImages = false; | ||
| expect(rulesAsStrings(sut), [ | ||
| ...alwaysEnabledRules, | ||
| ..._maybeWithSensitiveContent(), | ||
| 'SentryMaskingCustomRule<Widget>(Debug-mode-only warning for potentially sensitive widgets.)' | ||
| ]); | ||
| }); | ||
|
|
||
| test( | ||
| 'SensitiveContent rule is automatically added when current Flutter version is equal or newer than 3.33', | ||
| () { | ||
| final sut = SentryPrivacyOptions(); | ||
| final version = FlutterVersion.version!; | ||
| final dot = version.indexOf('.'); | ||
| final major = int.tryParse(version.substring(0, dot)); | ||
| final nextDot = version.indexOf('.', dot + 1); | ||
| final minor = int.tryParse( | ||
| version.substring(dot + 1, nextDot == -1 ? version.length : nextDot)); | ||
|
|
||
| if (major! > 3 || (major == 3 && minor! >= 33)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Version Parsing Flaw in Test CodeThe version parsing logic in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From above, if we create a helepr and test it popertly we can use it here instead of duplicating version parsing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah sorry, I think those are leftovers of when I did the testing, I'll properly expose the function instead |
||
| expect( | ||
| rulesAsStrings(sut).contains( | ||
| 'SentryMaskingCustomRule<SensitiveContent>(Mask SensitiveContent widget.)'), | ||
| isTrue, | ||
| reason: 'Test failed with version: ${FlutterVersion.version}'); | ||
| } else { | ||
| expect( | ||
| rulesAsStrings(sut).contains( | ||
| 'SentryMaskingCustomRule<SensitiveContent>(Mask SensitiveContent widget.)'), | ||
| isFalse, | ||
| reason: 'Test failed with version: ${FlutterVersion.version}'); | ||
| } | ||
| }, skip: FlutterVersion.version == null); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Version Parsing Vulnerability in SensitiveContent RuleThe version parsing logic in the Additional Locations (1) |
||
|
|
||
| group('user rules', () { | ||
| final defaultRules = [ | ||
| ...alwaysEnabledRules, | ||
| 'SentryMaskingCustomRule<Image>(Mask all images except asset images.)', | ||
| 'SentryMaskingConstantRule<Text>(mask)', | ||
| 'SentryMaskingConstantRule<EditableText>(mask)', | ||
| 'SentryMaskingConstantRule<RichText>(mask)', | ||
| ..._maybeWithSensitiveContent(), | ||
| 'SentryMaskingCustomRule<Widget>(Debug-mode-only warning for potentially sensitive widgets.)' | ||
| ]; | ||
|
|
||
| test('mask() takes precedence', () { | ||
| final sut = SentryPrivacyOptions(); | ||
| sut.mask<Image>(); | ||
| expect(rulesAsStrings(sut), | ||
| ['SentryMaskingConstantRule<Image>(mask)', ...defaultRules]); | ||
| expect(rulesAsStrings(sut), [ | ||
| 'SentryMaskingConstantRule<Image>(mask)', | ||
| ...defaultRules, | ||
| ]); | ||
| }); | ||
|
|
||
| test('unmask() takes precedence', () { | ||
| final sut = SentryPrivacyOptions(); | ||
| sut.unmask<Image>(); | ||
| expect(rulesAsStrings(sut), | ||
| ['SentryMaskingConstantRule<Image>(unmask)', ...defaultRules]); | ||
| expect(rulesAsStrings(sut), [ | ||
| 'SentryMaskingConstantRule<Image>(unmask)', | ||
| ...defaultRules, | ||
| ]); | ||
| }); | ||
|
|
||
| test('are ordered in the call order', () { | ||
| var sut = SentryPrivacyOptions(); | ||
| sut.mask<Image>(); | ||
|
|
@@ -272,13 +312,14 @@ void main() async { | |
| ...defaultRules | ||
| ]); | ||
| }); | ||
|
|
||
| test('maskCallback() takes precedence', () { | ||
| final sut = SentryPrivacyOptions(); | ||
| sut.maskCallback( | ||
| (Element element, Image widget) => SentryMaskingDecision.mask); | ||
| expect(rulesAsStrings(sut), [ | ||
| 'SentryMaskingCustomRule<Image>(Custom callback-based rule (description unspecified))', | ||
| ...defaultRules | ||
| ...defaultRules, | ||
| ]); | ||
| }); | ||
| test('User cannot add $SentryMask and $SentryUnmask rules', () { | ||
|
|
@@ -320,6 +361,25 @@ void main() async { | |
| }); | ||
| } | ||
|
|
||
| List<String> _maybeWithSensitiveContent() { | ||
| final version = FlutterVersion.version; | ||
| if (version == null) { | ||
| return []; | ||
| } | ||
| final dot = version.indexOf('.'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, duplicated logic for the third time. |
||
| final major = int.tryParse(version.substring(0, dot)); | ||
| final nextDot = version.indexOf('.', dot + 1); | ||
| final minor = int.tryParse( | ||
| version.substring(dot + 1, nextDot == -1 ? version.length : nextDot)); | ||
| if (major! > 3 || (major == 3 && minor! >= 33)) { | ||
| return [ | ||
| 'SentryMaskingCustomRule<SensitiveContent>(Mask SensitiveContent widget.)' | ||
| ]; | ||
| } else { | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| extension on Element { | ||
| Element findFirstOfType<T>() { | ||
| late Element result; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## 9.8.0.Consider moving the entry to the
## Unreleasedsection, please.