Skip to content

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Jun 13, 2025

📜 Description

Masks the SensitiveContent widget ootb for replay

💡 Motivation and Context

Closes #2987

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link

codecov bot commented Jun 13, 2025

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.48%. Comparing base (0929dbf) to head (1f8bf63).

Files with missing lines Patch % Lines
...ckages/flutter/lib/src/sentry_privacy_options.dart 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2989      +/-   ##
==========================================
+ Coverage   87.66%   89.48%   +1.81%     
==========================================
  Files         291       96     -195     
  Lines        9885     3442    -6443     
==========================================
- Hits         8666     3080    -5586     
+ Misses       1219      362     -857     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented Jul 11, 2025

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1250.02 ms 1271.02 ms 21.00 ms
Size 7.86 MiB 9.54 MiB 1.69 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
765aa8b 1259.09 ms 1269.90 ms 10.82 ms
b6c8720 1252.65 ms 1266.61 ms 13.96 ms
575ebaa 1262.20 ms 1274.24 ms 12.04 ms
ec78888 1251.37 ms 1269.40 ms 18.04 ms
cc4e375 1253.06 ms 1263.81 ms 10.75 ms
640ad0c 1241.04 ms 1253.96 ms 12.92 ms
793f4dc 1262.50 ms 1282.35 ms 19.85 ms
4481076 1256.48 ms 1266.64 ms 10.17 ms
2d34233 1258.19 ms 1268.92 ms 10.73 ms
f761369 1261.69 ms 1277.82 ms 16.12 ms

App size

Revision Plain With Sentry Diff
765aa8b 7.86 MiB 9.44 MiB 1.58 MiB
b6c8720 7.86 MiB 9.44 MiB 1.58 MiB
575ebaa 7.86 MiB 9.44 MiB 1.58 MiB
ec78888 7.86 MiB 9.44 MiB 1.58 MiB
cc4e375 7.86 MiB 9.44 MiB 1.58 MiB
640ad0c 7.86 MiB 9.44 MiB 1.58 MiB
793f4dc 7.86 MiB 9.44 MiB 1.58 MiB
4481076 7.86 MiB 9.44 MiB 1.58 MiB
2d34233 7.86 MiB 9.44 MiB 1.58 MiB
f761369 7.86 MiB 9.44 MiB 1.58 MiB

Previous results on branch: replay/add-sensitive-content

Startup times

Revision Plain With Sentry Diff
0c59077 1252.77 ms 1271.38 ms 18.61 ms
20657ad 1271.84 ms 1285.37 ms 13.53 ms
088cf62 1266.14 ms 1285.35 ms 19.20 ms
32e3fc4 1270.65 ms 1284.69 ms 14.04 ms
5ce2839 1271.55 ms 1281.62 ms 10.07 ms

App size

Revision Plain With Sentry Diff
0c59077 7.86 MiB 9.44 MiB 1.58 MiB
20657ad 7.86 MiB 9.44 MiB 1.58 MiB
088cf62 7.86 MiB 9.44 MiB 1.58 MiB
32e3fc4 7.86 MiB 9.44 MiB 1.58 MiB
5ce2839 7.86 MiB 9.44 MiB 1.58 MiB

Copy link
Contributor

github-actions bot commented Jul 11, 2025

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 493.32 ms 568.69 ms 75.37 ms
Size 6.54 MiB 7.70 MiB 1.17 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
93b7728 475.28 ms 489.13 ms 13.86 ms
0929dbf 462.82 ms 492.76 ms 29.94 ms
dbd526b 504.88 ms 569.02 ms 64.15 ms
e2d675d 457.92 ms 529.17 ms 71.25 ms
79f6b41 469.66 ms 525.90 ms 56.24 ms
b6c8720 457.41 ms 519.04 ms 61.63 ms
f761369 462.73 ms 563.80 ms 101.06 ms
2cf9161 454.12 ms 512.67 ms 58.55 ms
575ebaa 478.00 ms 585.76 ms 107.76 ms
73dca78 476.53 ms 522.21 ms 45.68 ms

App size

Revision Plain With Sentry Diff
93b7728 6.54 MiB 7.69 MiB 1.15 MiB
0929dbf 6.54 MiB 7.70 MiB 1.17 MiB
dbd526b 6.54 MiB 7.69 MiB 1.15 MiB
e2d675d 6.54 MiB 7.69 MiB 1.15 MiB
79f6b41 6.54 MiB 7.69 MiB 1.15 MiB
b6c8720 6.54 MiB 7.69 MiB 1.15 MiB
f761369 6.54 MiB 7.70 MiB 1.16 MiB
2cf9161 6.54 MiB 7.70 MiB 1.16 MiB
575ebaa 6.54 MiB 7.69 MiB 1.15 MiB
73dca78 6.54 MiB 7.69 MiB 1.15 MiB

Previous results on branch: replay/add-sensitive-content

Startup times

Revision Plain With Sentry Diff
20657ad 514.06 ms 607.15 ms 93.09 ms
088cf62 462.73 ms 544.39 ms 81.65 ms
0c59077 479.96 ms 570.00 ms 90.04 ms
32e3fc4 468.26 ms 538.44 ms 70.18 ms
5ce2839 462.72 ms 519.61 ms 56.89 ms

App size

Revision Plain With Sentry Diff
20657ad 6.54 MiB 7.70 MiB 1.16 MiB
088cf62 6.54 MiB 7.70 MiB 1.16 MiB
0c59077 6.54 MiB 7.70 MiB 1.16 MiB
32e3fc4 6.54 MiB 7.70 MiB 1.16 MiB
5ce2839 6.54 MiB 7.69 MiB 1.15 MiB

Copy link
Contributor

github-actions bot commented Jul 17, 2025

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

  • flutter/lib/src/screenshot/recorder.dart

@buenaflor buenaflor marked this pull request as ready for review July 29, 2025 14:37
@buenaflor buenaflor requested a review from stefanosiano as a code owner July 29, 2025 14:37
@buenaflor buenaflor requested a review from denrase as a code owner July 29, 2025 14:37
cursor[bot]

This comment was marked as outdated.

isFalse,
reason: 'Test failed with version: ${FlutterVersion.version}');
}
}, skip: FlutterVersion.version == null);
Copy link

Choose a reason for hiding this comment

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

Bug: Version Parsing Vulnerability in SensitiveContent Rule

The version parsing logic in the _maybeWithSensitiveContent() helper function and the SensitiveContent rule test case is unsafe. It can lead to a RangeError if the Flutter version string lacks a dot, and runtime exceptions if int.tryParse() returns null. This is due to missing boundary checks before substring() and reliance on null assertion operators (!) without prior null checks, contrasting with the main implementation's robust error handling.

Additional Locations (1)
Fix in Cursor Fix in Web

final minor = int.tryParse(
version.substring(dot + 1, nextDot == -1 ? version.length : nextDot));

if (major! > 3 || (major == 3 && minor! >= 33)) {
Copy link

Choose a reason for hiding this comment

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

Bug: Version Parsing Flaw in Test Code

The version parsing logic in masking_config_test.dart and its _maybeWithSensitiveContent helper function incorrectly uses non-null assertions (major!, minor!) on int.tryParse() results. This lack of null checking can lead to runtime exceptions for malformed version strings, a flaw not present in the main sentry_privacy_options.dart implementation.

Fix in Cursor Fix in Web

// Property not found – continue processing other rules.
return SentryMaskingDecision.continueProcessing;
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Masking Behavior Varies Between Modes

The maskSensitiveContent function exhibits inconsistent widget masking behavior between debug and release modes. In debug mode, widgets are only masked if their sensitivity property exists and is an Enum (due to an assert statement; otherwise, the try/catch prevents masking). In release mode, the assert is disabled, causing any widget with a sensitivity property to be masked, regardless of its type. This leads to different masking behavior between development and production environments, making testing unreliable.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SensitiveContent
2 participants