-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
Conversation
…dget is not masked
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2375 +/- ##
==========================================
+ Coverage 86.92% 91.48% +4.55%
==========================================
Files 264 87 -177
Lines 9376 3040 -6336
==========================================
- Hits 8150 2781 -5369
+ Misses 1226 259 -967 ☔ View full report in Codecov by Sentry. |
Android Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6e083bb | 327.96 ms | 381.39 ms | 53.43 ms |
4829ad3 | 381.55 ms | 455.45 ms | 73.90 ms |
9a604e8 | 470.65 ms | 485.90 ms | 15.24 ms |
a40bb7c | 477.10 ms | 553.90 ms | 76.80 ms |
0ceb89c | 304.57 ms | 357.18 ms | 52.61 ms |
ebfead1 | 298.44 ms | 374.28 ms | 75.84 ms |
f9d18f3 | 430.94 ms | 497.59 ms | 66.65 ms |
25161f4 | 353.98 ms | 431.94 ms | 77.96 ms |
732a7b4 | 371.98 ms | 423.19 ms | 51.21 ms |
d7758e8 | 300.12 ms | 349.88 ms | 49.76 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6e083bb | 5.94 MiB | 6.97 MiB | 1.03 MiB |
4829ad3 | 6.33 MiB | 7.26 MiB | 943.11 KiB |
9a604e8 | 6.49 MiB | 7.57 MiB | 1.08 MiB |
a40bb7c | 6.52 MiB | 7.61 MiB | 1.09 MiB |
0ceb89c | 5.94 MiB | 6.95 MiB | 1.01 MiB |
ebfead1 | 6.06 MiB | 7.03 MiB | 989.24 KiB |
f9d18f3 | 6.15 MiB | 7.13 MiB | 1000.49 KiB |
25161f4 | 6.27 MiB | 7.20 MiB | 960.44 KiB |
732a7b4 | 6.33 MiB | 7.27 MiB | 954.02 KiB |
d7758e8 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
Previous results on branch: feat/fail-unmasked-sensitive-widgets
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
905aa01 | 448.54 ms | 496.00 ms | 47.46 ms |
4708fe6 | 447.49 ms | 503.76 ms | 56.27 ms |
bb49dd5 | 523.42 ms | 570.79 ms | 47.37 ms |
1c71a86 | 478.22 ms | 521.78 ms | 43.55 ms |
16bd19d | 543.83 ms | 574.56 ms | 30.73 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
905aa01 | 6.49 MiB | 7.57 MiB | 1.08 MiB |
4708fe6 | 6.49 MiB | 7.57 MiB | 1.08 MiB |
bb49dd5 | 6.49 MiB | 7.57 MiB | 1.08 MiB |
1c71a86 | 6.49 MiB | 7.57 MiB | 1.08 MiB |
16bd19d | 6.49 MiB | 7.57 MiB | 1.08 MiB |
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
09c1f55 | 1258.11 ms | 1280.45 ms | 22.34 ms |
613760b | 1263.10 ms | 1277.27 ms | 14.16 ms |
2e93bab | 1237.08 ms | 1258.41 ms | 21.33 ms |
e0f6628 | 1250.57 ms | 1274.86 ms | 24.29 ms |
0a23f98 | 1252.98 ms | 1276.76 ms | 23.78 ms |
955541a | 1230.22 ms | 1252.96 ms | 22.73 ms |
6078ddc | 1207.84 ms | 1224.10 ms | 16.27 ms |
3f3ef0b | 1223.73 ms | 1237.67 ms | 13.94 ms |
3de8b9b | 1234.22 ms | 1251.94 ms | 17.72 ms |
3f23617 | 1261.93 ms | 1286.10 ms | 24.17 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
09c1f55 | 8.38 MiB | 9.74 MiB | 1.36 MiB |
613760b | 8.15 MiB | 9.13 MiB | 1000.46 KiB |
2e93bab | 8.38 MiB | 9.73 MiB | 1.36 MiB |
e0f6628 | 8.32 MiB | 9.50 MiB | 1.18 MiB |
0a23f98 | 8.10 MiB | 9.18 MiB | 1.08 MiB |
955541a | 8.28 MiB | 9.34 MiB | 1.06 MiB |
6078ddc | 8.33 MiB | 9.40 MiB | 1.07 MiB |
3f3ef0b | 8.32 MiB | 9.38 MiB | 1.05 MiB |
3de8b9b | 8.28 MiB | 9.34 MiB | 1.06 MiB |
3f23617 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
Previous results on branch: feat/fail-unmasked-sensitive-widgets
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bb49dd5 | 1249.61 ms | 1282.61 ms | 33.00 ms |
1c71a86 | 1259.45 ms | 1280.23 ms | 20.78 ms |
16bd19d | 1223.20 ms | 1234.47 ms | 11.27 ms |
905aa01 | 1245.10 ms | 1265.34 ms | 20.24 ms |
4708fe6 | 1261.65 ms | 1288.62 ms | 26.97 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bb49dd5 | 8.38 MiB | 9.75 MiB | 1.37 MiB |
1c71a86 | 8.38 MiB | 9.78 MiB | 1.41 MiB |
16bd19d | 8.38 MiB | 9.75 MiB | 1.37 MiB |
905aa01 | 8.38 MiB | 9.78 MiB | 1.40 MiB |
4708fe6 | 8.38 MiB | 9.75 MiB | 1.37 MiB |
🚨 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:
|
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.
looks good, just one question but not really blockign
|
||
// 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) { |
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.
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
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.
Thanks, I've made the changes and added test cases for non-debug builds
📜 Description
When a potentionally sensitive-content-containing widget is encountered and it is not masked (or unmasked explicitly), print a warning (in debug mode).
While testing this I noticed we don't have a TextField in tests which doesn't inherit from Text or FormField so added it although it's unrelated to this PR
💡 Motivation and Context
This should help users realize that they need to make adjustments to the privacy configuration. The code is only included in debug builds so it won't affect release builds in any way.
💚 How did you test it?
📝 Checklist
sendDefaultPii
is enabled🔮 Next steps